Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dashboard: Telemetry Settings Opt-In Checkbox #4330

Merged
merged 8 commits into from
Sep 7, 2020

Conversation

carloskelly13
Copy link
Contributor

@carloskelly13 carloskelly13 commented Sep 3, 2020

Summary

Add a checkbox for telemetry settings for opt-in for tracking

image

  • Focus support plus screen reader support
  • Connected to API and saves the state on check or uncheck

Relevant Technical Choices

Follows existing pattern of adding a setting to the settings page of a separate component.

To-do

None

User-facing changes

Testing Instructions

  1. Enable the Settings Page for Dashboard
  2. Visit Dashboard Settings
  3. Scroll to Data Sharing Opt-in
  4. Check it or un check it. See that your state is maintained on refresh.

The entire label should be clickable.

The Google Privacy link should open a new page but not change the state.


#3165

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2020

Size Change: +1.56 kB (0%)

Total Size: 1.24 MB

Filename Size Change
assets/js/stories-dashboard.js 584 kB +1.52 kB (0%)
ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 296 B +28 B (9%) 🔍
assets/css/stories-dashboard.css 328 B +23 B (7%) 🔍
assets/css/web-stories-embed-block.css 515 B 0 B
assets/js/chunk-web-stories-template-0-********************.js 10.2 kB 0 B
assets/js/chunk-web-stories-template-1-********************.js 10.3 kB 0 B
assets/js/chunk-web-stories-template-2-********************.js 10 kB 0 B
assets/js/chunk-web-stories-template-3-********************.js 10.5 kB 0 B
assets/js/chunk-web-stories-template-4-********************.js 10.6 kB 0 B
assets/js/chunk-web-stories-template-5-********************.js 6.86 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 9.91 kB 0 B
assets/js/chunk-web-stories-template-7-********************.js 9.78 kB 0 B
assets/js/edit-story.js 495 kB -12 B (0%)
assets/js/web-stories-activation-notice.js 63.6 kB 0 B
assets/js/web-stories-embed-block.js 16.9 kB 0 B

compressed-size-action

includes/Dashboard.php Outdated Show resolved Hide resolved
@carloskelly13
Copy link
Contributor Author

Updated screenshot since we need to split the sentence from the link.

image

@carloskelly13 carloskelly13 added this to the Sprint 36 milestone Sep 3, 2020
Copy link
Contributor

@BrittanyIRL BrittanyIRL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great - tested with keyboard + mouse on chrome, firefox, and safari.

There's one other part of the settings page that is going to need to be updated if I understand the discussion on #3165 correctly:

In this case, the settings page would be available to all users, but non-Admins should only be able to manage this setting (for now). The rest of the settings should only be visible to Admin users. I will update on the brief.

Right now, the settings route is only built if the enableSettingsFlag is true and canManageSettings from config is also true. Now that the specs on this page have changed we need to enable the route in dashboard/app/index and in the sidenav component if the enableSettingsFlag is true and then disable the analytics and file upload/ ability to edit logos if canManageSettings is false.

i feel like this would be a good separate PR.

Copy link
Contributor

@dmmulroy dmmulroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@swissspidy swissspidy removed the request for review from spacedmonkey September 4, 2020 09:45
@codecov
Copy link

codecov bot commented Sep 4, 2020

Codecov Report

Merging #4330 into main will increase coverage by 13.21%.
The diff coverage is 48.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #4330       +/-   ##
===========================================
+ Coverage   70.02%   83.23%   +13.21%     
===========================================
  Files         835      837        +2     
  Lines       14728    14809       +81     
===========================================
+ Hits        10313    12327     +2014     
+ Misses       4415     2482     -1933     
Flag Coverage Δ
#karmatests 71.24% <40.00%> (+41.88%) ⬆️
#unittests 66.24% <65.00%> (+0.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
includes/Dashboard.php 20.35% <0.00%> (-0.19%) ⬇️
assets/src/dashboard/app/api/useUserApi.js 69.56% <36.36%> (-30.44%) ⬇️
assets/src/dashboard/app/api/apiProvider.js 100.00% <100.00%> (ø)
...c/dashboard/app/views/editorSettings/components.js 100.00% <100.00%> (ø)
...ts/src/dashboard/app/views/editorSettings/index.js 52.45% <100.00%> (-21.36%) ⬇️
...hboard/app/views/editorSettings/telemetry/index.js 100.00% <100.00%> (ø)
bin/utils/generateZipFile.js 75.00% <0.00%> (-25.00%) ⬇️
includes/Story_Renderer/Embed.php 80.00% <0.00%> (-8.24%) ⬇️
...sets/src/dashboard/app/views/previewStory/index.js 85.18% <0.00%> (-8.00%) ⬇️
... and 205 more

@swissspidy swissspidy merged commit 51aeede into main Sep 7, 2020
@swissspidy swissspidy deleted the feature/dashboard-telemetry-setting branch September 7, 2020 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants